Hi @ikq,

JsSIP code must have the same style in order to make it readable and maintainable. Comments before making a deeper review:

• Debug line as the first method call in al public method implementations.
• New line after return.
• New line before and after call to super().
• First conditional in a block starts in new line and last ends in a new line.
• Conditional blocks always contain curly brackets.
• Comments in the line above when possible.
• Calls to debug have usually a new line above and a new line below.

Please take a certain file (ie: RTCSession.js) as a reference and try to stick to its style.

@ikq

Thanks for the cleanup, 👍 We'll review the PR as we can.

@ikq,

Cosmetic comment, we use camelCase in API method arguments (not in 100% of the cases, I know, but we will get there). Could you please make such change?

I mostly fixed the issues.

[I thought about dialog:
if you write in C++/java style, then you could add an abstract class dialog, and invite-dialog, subscriber and notifier dialogs would be his descendants.
The abstract dialog class could be something like stripped version of Dialog.js without actual method implementations]

About dialog, seems:

dialog interface:

• id (string)
• state (number)
• receiveRequest receive incoming request with the id

other functionality:

• check incoming requests
• keep set of parameters to create outgoing request.

It's already implemented in Subscriber/Notifier.
[Not exactly as in Dialog.js, but the same functionality]

Hi,

Super busy lately. Will come back to this when we have some time.

H @ikq,

Dialog should be agnostic to the method. Why can't current Dialog implementation be used here? It has basically the sendRequest() method that you can call, and it will call the receiveRequest() method on your Subscriber/Notifier implementation.

Hmm...
I checked the Dialog.js code again.
sendRequest and receiveRequest can be used.

I just did not consider this possibility before because I found INVITE/CANCEL/ACK specific code and properties in the Dialog (e.g. subscriber used different dialog states set).

There is INVITE related code in Dialog because a dialog for INVITE method needs to behave in certain way, different to generic dialogs, but that's an implementation detail. Other clases should be able to create dialogs the same way RTCSession does.

Hi Jose,
Now subscriber and notifier used Dialog.js
It passed my basic testing now.

I'll continue to test next week:

• I cannot check subscriber authentication (my sip server don't request it).
• subscriber with some production server.
• subscriber with notifier.

Testing of the current version has been completed successfully.
(subscribe authentication was not checked)

subscribe authentication checked using special test.
IMHO: the subscriber/notifier is ready.

I see I should change debug/debugerror to logger...

Hi @ikq,

We are super busy lately. Please keep using your branch and update this PR whenever you find any issue/enhancement.

Also, tests would accelerate the adoption.

I already use this code for my work and so far no problem.

Two browsers test can be taken from:
https://github.com/ikq/subscribe_notify_test

@ikq, more than an external test I mean tests in JsSIP as part of this PR, testing the clases created here.

Hi, is there some news? I'd like to test this functionality.

Hi, is there some news? I'd like to test this functionality.

IMHO it's stable code.
[At least we use subscriber in our production, currently no issues reported]

The subscriber/notifier test can taken from https://github.com/ikq/subscribe_notify_test

Hi, is there a specific reason why Subscriber and Notifier don't use the Contact header from the UA like RTCSession does? If the user provides a custom contact_uri in the UAConfiguration he would have to manually add it as a extra header to Subscriber and Notifier again, wouldn't he?

Hi, is there some news? I'd like to test this functionality.

IMHO it's stable code.
[At least we use subscriber in our production, currently no issues reported]

The subscriber/notifier test can taken from https://github.com/ikq/subscribe_notify_test

I'd like to use the official repository, so I'd like if it will be merged ASAP

Hi, is there a specific reason why Subscriber and Notifier don't use the Contact header from the UA like RTCSession does? If the user provides a custom contact_uri in the UAConfiguration he would have to manually add it as a extra header to Subscriber and Notifier again, wouldn't he?

Yes, I'm not using the Configuration.contact_uri

In fact, I was in a hurry to do this work, and did not pay attention at all to that there is such a parameter in the configuration !

Let check, how the configuration used for INVITE:

UA.js:
   this._contact = {
      pub_gruu  : null,
      temp_gruu : null,
      uri       : this._configuration.contact_uri,

RTCSession.js:
  this._contact = this._ua.contact.toString({
      anonymous,
      outbound : true
    });

Thanks.
It can be improved (use of Configuration.contact_uri)

P.S.
Usage Configuration.contact_uri will modify default SUBSCRIBE contact - will be missed "+sip.instance" (that I add to default SUBSCRIBE Contact)
User still can set "+sip.instance" to SUBSCRIBE Contact via custom Contact header.

@ikq if you don't mind external contributions i'd be willing to take a swing at writing unit tests for this project's test framework

if you don't mind external contributions i'd be willing to take a swing at writing unit tests for this project's test framework

Thank you, I will gladly accept your help.

I have provided an external test for 2 browsers: one is sending an SUBSCRIBE and the other is receiving it and sending a NOTIFY.

Seems in unit test should be simulated 2 instances of JsSIP one send/other receive.
[I don't know how it can be written.
Probably instead two JsSIP instances, can be used single that send the message itself ?]
;-)

Thanks.
It can be improved (use of Configuration.contact_uri)

P.S.
Usage Configuration.contact_uri will modify default SUBSCRIBE contact - will be missed "+sip.instance" (that I add to default SUBSCRIBE Contact)
User still can set "+sip.instance" to SUBSCRIBE Contact via custom Contact header.

@ikq Is there a specific reason to use Configuration.contact_uri? Why not just do it as in RTCSession, with this._ua.contact.toString? Then it would also use the gruu if one was provided. Or am I missing something?

Thank you markusatm for founding the problem.
Thank you stefang42, I use your fix to build Contact in subscriber and notifier.

Hi,

Yes, in order for this PR to be merged we need tests in JsSIP project.

You don't need two JsSIP instances, just one instance and you should inject a transport that you control (a FAKE transport) that you use to send JsSIP UA SUBSCRIBE|NOTIFY messages and receive and check what comes from JsSIP.

If you need some help please let us know.

Hi,
I added nodeunit test to test subscriber notifier communication.

1. Implemented loopSocket that send message itself, but modified Call-ID in each leg.
   (without the modification method findDialog confuses SUBSCRIBE dialog with NOTIFY dialog)

2. subscriber and notifier run in the same JsSIP instance and send/receive SIP messages.

The test SIP sequence is:

• SUBSCRIBE
• SUBSCRIBE OK
• NOTIFY
• NOTIFY OK
• unSUBSCRIBE (with expires 0)
• unSUBSCRIBE OK
• final NOTIFY (with Subscription-State: terminated)
• final NOTIFY OK.

The unit test work silently, but if uncomment line JsSIP.debug.enable('JsSIP:*');
can be seen complete SIP communication.

SUBSCRIBE and NOTIFY headers checking:
url, method, contact, subscribe accept, notify subscription-state, content-type and body.
Checked order of event sequence.

@jmillan We already using it this way :) but it's a bit difficult to maintain separate branch.
Anyways, thanks you!

I'm not forgetting about this...,

@jmillan Hello! Any chance you had time to review this one?
Thanks!

@jmillan Ping?

For those willing to use this feature I would suggest you use this branch. I don't have at the moment the resources to re-check and merge this, so please, do use this branch for the time being.

Looking forward with this merge also 👍

@jmillan ping?

Pushed a rebased version in #922.

@ikq thanks for the patience. I'll try 🤞 to finish the review and accept it in the following weeks. Can I ask you to not include any change in dist/ folder nor in package.json and to sync the branch with master?

Also please do not edit CHANGELOG.md

@ikq thanks for the patience. I'll try 🤞 to finish the review and accept it in the following weeks. Can I ask you to not include any change in dist/ folder nor in package.json and to sync the branch with master?

Done

Hi @jmillan. Thanks for the review.

I work with @ikq at Audiocodes, and he agreed to let me take over this change as he has no time to address all the comments.

I pushed a new rebased revision, hopefully addressing all your comments. Please review.

Thanks, I'll take some time on a further, and hopefully last review.

Currently UA fires newSubscribe with the incoming SUBSCRIBE request.

If SUBSCRIBE request does not have a Contact header, the request WONT be responded, hence breaking the SIP protocol. Contact header is verified in Notifier constructor which simply throws if it's missing. This kind of sanity checks should be done in Notifier.init_incoming(subscriber) which should reject the request or emit newSubscribe. This method can be a static method in Notifier.

Check a similar case in RtcSession: https://github.com/versatica/JsSIP/blob/master/lib/RTCSession.js#L3499

@orgads, there may be some duplicated suggestion on my review due to a problem between GH and my side, sorry for that. Please simply resolve those duplicated.

I think all the comments should be now covered.

(I can't Resolve comments since I'm not the owner 🙄)

(I can't Resolve comments since I'm not the owner 🙄)

I will, no worries.